Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JTC] Process tolerances sent with action goal (backport #716) #1189

Merged
merged 4 commits into from
Jul 15, 2024
Merged

[JTC] Process tolerances sent with action goal (backport #716) #1189

merged 4 commits into from
Jul 15, 2024

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Jul 3, 2024

From here:

JointTrajectoryController ignores goal_time_tolerance set via FollowJointTrajectory.Goal.goal_time_tolerance together with path_tolerance and goal_tolerance .

This PR proposes a way how to process the tolerances: The tolerances from one action goal are not saved for the next one, but the default ones will be used if no tolerances are set with the following action goals.

(Temporary) deactivation is also implemented like documented in the msg definition.

From the node parameter we cannot set velocity or acceleration tolerances (except for a single stopped_velocity_tolerance for the goal tolerances of all joints). Should we add them as parameters as well, to have the same structure like the action message?

This new feature could break some existing projects, because the tolerances were just ignored and might now be breaking behaviors. Should we introduce a temporary parameter to opt-in?

Fixes #249

This is an automatic backport of pull request #716 done by Mergify.

@christophfroehlich @mergify
[JTC] Process tolerances sent with action goal (#716)
d128129 
(cherry picked from commit 07061f9)

# Conflicts:
#	doc/migration/Jazzy.rst
#	doc/release_notes/Jazzy.rst
@mergify mergify bot added the conflicts label Jul 3, 2024
@mergify Mergify
Copy link
Contributor Author

mergify bot commented Jul 3, 2024

Cherry-pick of 07061f9 has failed:

On branch mergify/bp/humble/pr-716
Your branch is up to date with 'origin/humble'.

You are currently cherry-picking commit 07061f9.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   joint_trajectory_controller/CMakeLists.txt
	modified:   joint_trajectory_controller/doc/userdoc.rst
	modified:   joint_trajectory_controller/include/joint_trajectory_controller/joint_trajectory_controller.hpp
	modified:   joint_trajectory_controller/include/joint_trajectory_controller/tolerances.hpp
	modified:   joint_trajectory_controller/src/joint_trajectory_controller.cpp
	new file:   joint_trajectory_controller/test/test_tolerances.cpp
	modified:   joint_trajectory_controller/test/test_trajectory_actions.cpp
	modified:   joint_trajectory_controller/test/test_trajectory_controller_utils.hpp

Unmerged paths:
  (use "git add/rm <file>..." as appropriate to mark resolution)
	deleted by us:   doc/migration/Jazzy.rst
	deleted by us:   doc/release_notes/Jazzy.rst

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@codecov Codecov
Copy link

codecov bot commented Jul 3, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 88.67925% with 48 lines in your changes missing coverage. Please review.

Project coverage is 86.74%. Comparing base (0766dac) to head (29888d8).

Additional details and impacted files 
@@            Coverage Diff             @@
##           humble    #1189      +/-   ##
==========================================
- Coverage   86.98%   86.74%   -0.25%     
==========================================
  Files          86       87       +1     
  Lines        7490     7904     +414     
  Branches      617      628      +11     
==========================================
+ Hits         6515     6856     +341     
- Misses        743      805      +62     
- Partials      232      243      +11
Flag Coverage Δ
unittests 86.74% <88.67%> (-0.25%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...jectory_controller/joint_trajectory_controller.hpp 100.00% <ø> (ø)
...int_trajectory_controller/test/test_tolerances.cpp 100.00% <100.00%> (ø)
...ectory_controller/test/test_trajectory_actions.cpp 97.75% <100.00%> (+0.47%) ⬆️
...ory_controller/src/joint_trajectory_controller.cpp 84.03% <93.75%> (+0.05%) ⬆️
...ntroller/test/test_trajectory_controller_utils.hpp 84.59% <23.07%> (-6.06%) ⬇️
...include/joint_trajectory_controller/tolerances.hpp 69.90% <63.01%> (-17.20%) ⬇️

... and 2 files with indirect coverage changes

@christophfroehlich
Copy link
Contributor

I fixed the merge conflict with the release notes, and removed all entries which are not applicable to this branch.

@christophfroehlich christophfroehlich mentioned this pull request Jul 15, 2024
Merged
@christophfroehlich
Copy link
Contributor

Needs ros-controls/ros2_control#1613 for Humble Check Docks job.

@bmagyar bmagyar merged commit 112df35 into humble Jul 15, 2024
10 of 12 checks passed
@bmagyar bmagyar deleted the mergify/bp/humble/pr-716 branch July 15, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bmagyar bmagyar bmagyar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@christophfroehlich @bmagyar